Skip to content

Wrap osc8 to make the url clickable in terminal - #1699

Open
phanen wants to merge 1 commit into
librespot-org:devfrom
phanen:osc8
Open

Wrap osc8 to make the url clickable in terminal#1699
phanen wants to merge 1 commit into
librespot-org:devfrom
phanen:osc8

Conversation

@phanen

@phanen phanen commented Mar 23, 2026

Copy link
Copy Markdown

Copilot AI review requested due to automatic review settings March 23, 2026 14:11

This comment was marked as outdated.

@phanen
phanen force-pushed the osc8 branch 2 times, most recently from 7ae51a1 to ff57d50 Compare March 23, 2026 14:20
@m00grin

m00grin commented Sep 2, 2026

Copy link
Copy Markdown

Rebased this against current dev. Applies cleanly, no changes needed.
fmt, clippy -D warnings, and tests all pass. Small and isolated, still
good to go.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new output emits raw OSC 8 control sequences unconditionally and without sanitization, which can break redirected output and introduces terminal escape injection risk.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

oauth/src/lib.rs:470

  • This prints OSC 8 escape sequences unconditionally. When stdout is redirected (logs, pipes) or on terminals without OSC 8 support, this will emit raw control characters. Also, if the URL ever contains control chars (e.g., ESC/BEL), it can break out of the hyperlink sequence and potentially inject terminal control codes; sanitizing and only emitting OSC 8 when stdout is a terminal avoids that.
    println!("Browse to: \x1b]8;;{auth_url}\x1b\\{auth_url}\x1b]8;;\x1b\\",);
  • Files reviewed: 1/1 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread oauth/src/lib.rs Outdated
@kingosticks

Copy link
Copy Markdown
Member

While it rebases fine but, it's missing support for the latest stuff at

println!("Browse to: {url}");

@kingosticks

Copy link
Copy Markdown
Member

@copilot Add a changelog entry for this PR in CHANGELOG.md

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants